Skip to content

Allow ignoring cookie exception #2297

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Nov 12, 2025
Merged

Allow ignoring cookie exception #2297

merged 7 commits into from
Nov 12, 2025

Conversation

@alexeyzimarev
Copy link
Member

@alexeyzimarev alexeyzimarev commented Sep 11, 2025

Description

Fixes #2292

Purpose

This pull request is a:

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Needs tests

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Sep 11, 2025 •
edited
Loading

Deploying restsharp with  Cloudflare Pages  Cloudflare Pages

Latest commit: 12fb325
Status: ✅  Deploy successful!
Preview URL: https://7f738e5d.restsharp.pages.dev
Branch Preview URL: https://ignore-invalid-cookies.restsharp.pages.dev

View logs

@github-actions
Copy link

github-actions bot commented Sep 11, 2025 •
edited
Loading

Test Results

   35 files     35 suites   15m 30s ⏱️
  453 tests   453 ✅ 0 💤 0 ❌
3 062 runs  3 062 ✅ 0 💤 0 ❌

Results for commit 12fb325.

♻️ This comment has been updated with latest results.

@alexeyzimarev
Copy link
Member Author

alexeyzimarev commented Nov 10, 2025

/review

@qodo-merge-for-open-source
Copy link

qodo-merge-for-open-source bot commented Nov 10, 2025 •
edited
Loading

PR Reviewer Guide 🔍

(Review updated until commit 316baf6)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅

2292 - PR Code Verified

Compliant requirements:

  • Added a boolean configuration option (IgnoreInvalidCookies in RestClientOptions)
  • Implemented check before adding cookies in RestClient.Async.cs
  • Wrapped cookie addition in try-catch to handle CookieException when option is enabled
  • Allows accepting request data even when invalid cookies are present

Requires further human verification:

  • Verify that the solution handles the specific domain mismatch scenario described in the ticket (api.new-domain.com returning cookies for .old-domain.com)
  • Confirm that the response data is properly returned when invalid cookies are ignored
  • Test that valid cookies are still processed correctly when IgnoreInvalidCookies is false
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Missing Return Statement

The code removes the return statement from inside the catch block but doesn't show what happens after the catch. The method needs to return an HttpResponse in all code paths, but it's unclear how the method handles the case when an exception occurs and IgnoreInvalidCookies is false.

 
catch (Exception ex) {
    return new(null, url, null, ex, timeoutCts.Token);
}
Incomplete Exception Handling

The catch block at line 161 catches all exceptions but only returns an error response. This doesn't distinguish between cookie-related exceptions and other HTTP exceptions. If a non-cookie exception occurs, the code will still try to process cookies and execute the after-request hooks, which may fail or produce unexpected behavior.

 
        catch (Exception ex) {
            return new(null, url, null, ex, timeoutCts.Token);
        }

#pragma warning disable CS0618 // Type or member is obsolete
        if (request.OnAfterRequest != null) await request.OnAfterRequest(responseMessage).ConfigureAwait(false);
#pragma warning restore CS0618 // Type or member is obsolete
        await OnAfterHttpRequest(request, responseMessage, cancellationToken).ConfigureAwait(false);
        return new(responseMessage, url, cookieContainer, null, timeoutCts.Token);

@alexeyzimarev
Copy link
Member Author

alexeyzimarev commented Nov 12, 2025

/review

@qodo-merge-for-open-source
Copy link

qodo-merge-for-open-source bot commented Nov 12, 2025

Persistent review updated to latest commit 316baf6

@alexeyzimarev alexeyzimarev merged commit b034cda into dev Nov 12, 2025
11 checks passed
@alexeyzimarev alexeyzimarev deleted the ignore-invalid-cookies branch November 12, 2025 15:05
@qodo-merge-for-open-source
Copy link

qodo-merge-for-open-source bot commented Nov 12, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡
🎫 #2292
🔴 v
C
E
x
q
b
:
R
W
y
⚪ -
H
a
n
d
l
e
t
h
s
p
c
i
f
u
w
r
A
P
I
o
k
m
(
.
g
,
)
Codebase Duplication Compliance
⚪
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Review effort 3/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

option to ignore response cookies

2 participants

@alexeyzimarev